[APM] Critical path for a single trace #143735

dgieselaar · 2022-10-20T10:45:14Z

…review

dgieselaar · 2022-10-23T15:10:36Z

x-pack/plugins/apm/common/critical_path/get_critical_path.ts

+          (
+            normalizedChildStart >= scanTime ||
+            normalizedChildEnd < start ||
+            childEnd > scanTime


@AlexanderWert do you remember what the reasoning was here? I understand that a span cannot be on the critical path if it ends after its parent, because it implies that the parent doesn't care about it finishing, but this also eliminates spans that end after its sibling that was previously on the critical path. Which might be a valid reason, but wanted to clarify that with you.

Let's take this one as an example:

The algorithm starts from the end with GET /api/order and we know that GET /api/order is on the CP. The spans Parse the document and Longtask are partially parallel to GET /api/order because they are not a parent of GET /api/order, started before GET /api/order, but DID NOT END before GET /api/order. This implies that the start of GET /api/oder is blocked by its parent rather one of the other two mentioned spans. As a consequence, the CP before GET /api/order lies on the parent orders and we can eliminate Parce the document and Longtask from the critical path.

Thank you, that makes sense!

elasticmachine · 2022-10-24T06:12:00Z

Pinging @elastic/apm-ui (Team:APM)

…review

afharo

Core Changes LGTM

cauemarcondes · 2022-10-26T13:07:27Z

I think a Switch would look better in this case, since you are turning it on and off. You could even change the text to show/hide based on the status.

I also think using the tech preview icon instead of the text would look nicer. Especially because when I first look at this my first impression is that the entire waterfall is in tech preview,

cc: @boriskirov

cauemarcondes · 2022-10-26T13:11:24Z

@dgieselaar is there any documentation we should point the users to? Is it clear what a critical path is for everyone?

cauemarcondes

a few nits let me know what you think.

cauemarcondes · 2022-10-26T13:15:53Z

packages/kbn-apm-synthtrace/src/lib/apm/instance.ts

    });
  }

-  span({ spanName, spanType, spanSubtype, ...apmFields }: SpanParams) {
+  span(...options: [string, string] | [string, string, string] | [SpanParams]) {


This looks very weird, why is it needed?

it's basically method overloading, but in one signature. I think span(spanName) is often good enough and more concise than span({ spanName: ... }). I previously discussed this with @sqren when he changed it, and IIRC(?) we decided I would add back the original method when I would have time.

I agree it makes the signature simpler but I'd rather be without the extra complexity in synthtrace. But I don't feel strongly for it either way.

Actually, I'll take that back. I don't think the signature improved except for transaction() with a single arg.

I think the signature is more complicated but not less easy to use, which is my goal. Like you know, I disagree about span({...}) and transaction({...}) being the better interface. I do we think should add more defaults for span() though, IMHO span(spanName) is good enough too, we can default to custom/custom for span type and subtype.

cauemarcondes · 2022-10-26T13:22:17Z

packages/kbn-apm-synthtrace/src/lib/apm/service.ts

+export function service(name: string, environment: string, agentName: string): Service;
+
+export function service(options: { name: string; environment: string; agentName: string }): Service;
+
+export function service(
+  ...args: [{ name: string; environment: string; agentName: string }] | [string, string, string]
+) {
+  const [serviceName, environment, agentName] =
+    args.length === 1 ? [args[0].name, args[0].environment, args[0].agentName] : args;


Doesn't this overcomplicate this a bit? Is it really necessary?

See comment above.

cauemarcondes · 2022-10-26T13:24:43Z

x-pack/plugins/observability/server/ui_settings.ts

+    description: i18n.translate('xpack.observability.enableCriticalPathDescription', {
+      defaultMessage: '{technicalPreviewLabel} Optionally display the critical path of a trace.',
+      values: {
+        technicalPreviewLabel: `<em>[${technicalPreviewLabel}]</em>`,
+      },
+    }),


@boriskirov we're showing a link where users can give feedback for tech preview features, isn't there a link for this as well?

@AlexanderWert how do you feel about this?

cauemarcondes · 2022-10-26T13:27:51Z

x-pack/plugins/apm/common/critical_path/types.ts

+ * 2.0.
+ */
+
+import type { IWaterfallSpanOrTransaction } from '../../public/components/app/transaction_details/waterfall_with_summary/waterfall_container/waterfall/waterfall_helpers/waterfall_helpers';


Doesn't this look odd a common file importing stuff from a public component? Can you move the typing definitions of the waterfall to a common folder and use it here and adjust the imports? That's already a starting point to refactor the waterfall, we discussed about moving some of the logic to the server a couple of weeks ago.

This is a type-only import which I think is usually fine. I get your point, but I think there's also a lot of value of co-locating types with the code that uses them (the most).

cauemarcondes · 2022-10-26T13:31:15Z

x-pack/plugins/apm/common/critical_path/get_critical_path.ts

+          // the critical path, but we only want one to contribute.
+          // - The span/tx ends before the start of the initial scan period.
+          // - The span ends _after_ the current scan time.
+          (


can you remove this extra (?

yes, I'm actually surprised ESLint doesn't remove this. Maybe it adds it automatically because of the comment, I'll look into it.

ESLint adds it automatically :)

I think if you move the comments up it'll look nicer. but it's up to you.

cauemarcondes · 2022-10-26T13:40:29Z

x-pack/plugins/apm/public/hooks/use_critical_path_enabled_setting.ts

+import { enableCriticalPath } from '@kbn/observability-plugin/common';
+import { useApmPluginContext } from '../context/apm_plugin/use_apm_plugin_context';
+
+export function useCriticalPathEnabledSetting() {


Suggested change

export function useCriticalPathEnabledSetting() {

export function useCriticalPathSetting() {

I want to distinguish between whether the feature is enabled and whether it is toggled on in the current view. Not sure if this is the best name but it's better than useCriticalPathSetting IMO.

If you change it to align the settings name as discussed below I think it'll be fine: useCriticalPathFeatureEnabledSetting

cauemarcondes · 2022-10-26T13:42:48Z

x-pack/plugins/apm/public/components/routing/service_detail/index.tsx

+            defaults: {
+              query: {
+                showCriticalPath: '',
+              },
+            },


Why is it necessary if you'll use the value defined in the settings?

In some cases we cannot type-check links (e.g. when linking from outside the app), this is to handle those cases.

cauemarcondes · 2022-10-26T13:58:07Z

...saction_details/waterfall_with_summary/waterfall_container/waterfall/accordion_waterfall.tsx

+  const criticalPath = showCriticalPath
+    ? getCriticalPath(waterfall)
+    : undefined;
+
+  const criticalPathSegmentsById = groupBy(
+    criticalPath?.segments,
+    (segment) => segment.item.id
+  );


Do you think it's important to wrap it inside a useMemo so it won't recalculate on every render?

I think this will be fast enough. There will only be a handful of segments in most cases.

cauemarcondes · 2022-10-26T14:31:02Z

x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx

+          onShowCriticalPathChange={(nextShowCriticalPath) => {
+            push(history, {
+              query: {
+                showCriticalPath: nextShowCriticalPath ? 'true' : 'false',


Can you use toString instead?

Suggested change

showCriticalPath: nextShowCriticalPath ? 'true' : 'false',

showCriticalPath: nextShowCriticalPath.toString(),

I see your point, but I think I have a slight preference for being explicit here.

cauemarcondes · 2022-10-26T14:34:27Z

...blic/components/app/transaction_details/waterfall_with_summary/waterfall_container/index.tsx

-      <Waterfall waterfallItemId={waterfallItemId} waterfall={waterfall} />
-    </div>
+    <EuiFlexGroup direction="column">
+      {isCriticalPathEnabled ? (


I think the name of the setting and the props are a bit confusing. I think the other way around would make more sense.

Change the settings to showCriticalPath and change the props to enableCriticalPath.

Not sure about swapping them, I think I'll change it to isCriticalPathFeatureEnabled or maybe isCriticalPathFeatureAvailable. WDYT?

I think isCriticalPathFeatureEnabled is better.

…review

…into critical-path-preview

dgieselaar · 2022-10-27T17:46:54Z

@cauemarcondes I think I got most of it, mind having another look?

cauemarcondes

LGTM

cauemarcondes · 2022-10-27T18:49:36Z

x-pack/plugins/apm/common/critical_path/get_critical_path.ts

+          // the critical path, but we only want one to contribute.
+          // - The span/tx ends before the start of the initial scan period.
+          // - The span ends _after_ the current scan time.
+          (


I think if you move the comments up it'll look nicer. but it's up to you.

kibana-ci · 2022-10-27T18:58:43Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1336	1338	+2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/apm-synthtrace`	74	76	+2
`observability`	536	546	+10
total			+12

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	3.1MB	3.1MB	+3.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`observability`	68.0KB	68.2KB	+127.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/apm-synthtrace`	74	76	+2
`observability`	540	550	+10
total			+12

History

💚 Build #82720 succeeded 3d1787d
💚 Build #82022 succeeded 7fb95b8
💔 Build #82001 failed d01ede8
💔 Build #81728 failed 0b61ad1

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2022-10-27T19:40:09Z

💔 All backports failed

Status	Branch	Result
❌	8.5	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 143735

Questions ?

Please refer to the Backport tool documentation

* main: (24 commits) [Files] Add file upload to file picker (elastic#143969) [Security solution] Guided onboarding, alerts & cases (elastic#143598) [APM] Critical path for a single trace (elastic#143735) skip failing test suite (elastic#143933) [Fleet] Update GH Projects automation (elastic#144123) [APM] Performance fix for 'cardinality' telemetry task (elastic#144061) [Enterprise Search] Attach ML Inference Pipeline - Pipeline re-use (elastic#143979) [8.5][DOCS] Add support for differential logs (elastic#143242) Bump nwsapi from v2.2.0 to v2.2.2 (elastic#144001) [APM] Add waterfall to dependency operations (elastic#143257) [Shared UX] Add deprecation message to kibana react Markdown (elastic#143766) [Security Solution][Endpoint] Adds RBAC API checks for Blocklist (elastic#144047) Improve `needs-team` auto labeling regex (elastic#143787) [Reporting/CSV Export] _id field can not be formatted (elastic#143807) Adds SavedObjectsWarning to analytics results pages. (elastic#144109) Bump chromedriver to 107 (elastic#144073) Update cypress (elastic#143755) [Maps] nest security layers in layer group (elastic#144055) [Lens][Agg based Heatmap] Navigate to Lens Agg based Heatmap. (elastic#143820) Added support of saved search (elastic#144095) ...

kibanamachine · 2022-10-28T19:41:43Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 143735 locally

kibanamachine · 2022-10-31T19:51:42Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 143735 locally

sorenlouv · 2022-10-31T20:41:41Z

x-pack/plugins/apm/common/critical_path/get_critical_path.test.ts

+        .timestamp(1)
+        .duration(100)
+        .children(
+          service.span('foo', 'external', 'db').duration(100).timestamp(1)


I actually think this reads worse than before. I'm not sure what either of those arguments are.

I disagree, it's clear to me. I realise I'm biased as the author but I think you're also exaggerating a little bit here :).

I disagree, it's clear to me

How many on the team do you think will know that "foo" is span.name, "external" is span.type and "db" is span.subtype?

If it's more than half I'll shut up. We should do a pop quiz and settle this ;)

dgieselaar added 5 commits October 19, 2022 13:13

[APM] Critical path for a single trace

120c5e0

Merge branch 'main' of github.com:elastic/kibana into critical-path-p…

22baeb1

…review

Merge branch 'main' of github.com:elastic/kibana into critical-path-p…

8c28f3c

…review

Add tech preview badge

96ea7da

Merge branch 'main' of github.com:elastic/kibana into critical-path-p…

0b61ad1

…review

dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v8.5.0 labels Oct 20, 2022

dgieselaar requested a review from AlexanderWert October 20, 2022 10:45

dgieselaar added 2 commits October 23, 2022 17:05

Update synthtrace tests

aaae194

Merge branch 'main' of github.com:elastic/kibana into critical-path-p…

d01ede8

…review

dgieselaar commented Oct 23, 2022

View reviewed changes

dgieselaar marked this pull request as ready for review October 23, 2022 15:10

dgieselaar requested a review from a team as a code owner October 23, 2022 15:10

botelastic bot added the Team:APM All issues that need APM UI Team support label Oct 24, 2022

dgieselaar added 3 commits October 24, 2022 09:34

Add new setting to mapping

1c91019

Merge branch 'main' of github.com:elastic/kibana into critical-path-p…

cded11e

…review

Make sure timestamp.us is set for error events as well

7fb95b8

dgieselaar requested review from a team as code owners October 24, 2022 08:14

afharo approved these changes Oct 24, 2022

View reviewed changes

Merge branch 'main' into critical-path-preview

3d1787d

cauemarcondes reviewed Oct 26, 2022

View reviewed changes

dgieselaar added 3 commits October 27, 2022 19:45

Merge branch 'main' of github.com:elastic/kibana into critical-path-p…

0e6919e

…review

Merge branch 'main' of github.com:elastic/kibana into critical-path-p…

06dd608

…review

Merge branch 'critical-path-preview' of github.com:dgieselaar/kibana …

cb4d9b5

…into critical-path-preview

cauemarcondes approved these changes Oct 27, 2022

View reviewed changes

dgieselaar merged commit 4bd8693 into elastic:main Oct 27, 2022

dgieselaar deleted the critical-path-preview branch October 27, 2022 19:33

kibanamachine added the v8.6.0 label Oct 27, 2022

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 28, 2022

sorenlouv reviewed Oct 31, 2022

View reviewed changes

dgieselaar removed the v8.5.0 label Nov 1, 2022

kibanamachine added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Nov 1, 2022

cauemarcondes mentioned this pull request Nov 28, 2022

[DOCS] Adds the 8.6.0 release notes #145768

Merged

	export function useCriticalPathEnabledSetting() {
	export function useCriticalPathSetting() {

	showCriticalPath: nextShowCriticalPath ? 'true' : 'false',
	showCriticalPath: nextShowCriticalPath.toString(),

[APM] Critical path for a single trace #143735

[APM] Critical path for a single trace #143735

Conversation

dgieselaar commented Oct 20, 2022 • edited by kibanamachine Loading

Choose a reason for hiding this comment

AlexanderWert Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 24, 2022

afharo left a comment

Choose a reason for hiding this comment

cauemarcondes commented Oct 26, 2022 • edited Loading

cauemarcondes commented Oct 26, 2022

cauemarcondes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cauemarcondes Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar commented Oct 27, 2022

cauemarcondes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Oct 27, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

kibanamachine commented Oct 27, 2022

💔 All backports failed

Manual backport

Questions ?

kibanamachine commented Oct 28, 2022

kibanamachine commented Oct 31, 2022

sorenlouv Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar commented Oct 20, 2022 •

edited by kibanamachine

Loading

AlexanderWert Oct 24, 2022 •

edited

Loading

cauemarcondes commented Oct 26, 2022 •

edited

Loading

cauemarcondes Oct 26, 2022 •

edited

Loading

sorenlouv Oct 31, 2022 •

edited

Loading